-
Notifications
You must be signed in to change notification settings - Fork 30
Various fixes for propagating CachedArrayStyle correctly #391
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #391 +/- ##
==========================================
- Coverage 95.94% 95.76% -0.19%
==========================================
Files 17 17
Lines 3376 3467 +91
==========================================
+ Hits 3239 3320 +81
- Misses 137 147 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Similar cases missing a julia> v = Accumulate(*, 1:10);
julia> (Base.BroadcastStyle ∘ typeof).((v, unitblocks(v), BlockBroadcastArray(vcat, unitblocks(v), unitblocks(v))))
(LazyArrays.CachedArrayStyle{1}(), BlockArrays.BlockedStyle{1}(), LazyArrays.LazyArrayStyle{1}())So I might have to eventually go and fix these, assuming it would be appropriate that these all return julia> Base.BroadcastStyle(typeof(vec(Vcat(v', v')))) # interlaced
Base.Broadcast.DefaultArrayStyle{1}()will get a fix (this last one is now fixed in JuliaLinearAlgebra/ArrayLayouts.jl#268) |
This allows e.g.
```julia-repl
julia> v = Accumulate(*, 1:10); Base.BroadcastStyle(typeof(vec(Vcat(v', v'))))
LazyArrays.LazyArrayStyle{2}()
```
(and, after JuliaArrays/LazyArrays.jl#391, this
would return `CachedArrayStyle{2}()`)
|
I guess we don't need a |
|
I think that's right. I wasn't too sure exactly about cases like that (and e.g. the block matrices like I mentioned in my previous comment), but I think I reasoned similarly that |
|
This BroadcastStyle(::Type{<:LinearAlgebra.QRCompactWYQ}) = DefaultArrayStyle{2}()
BroadcastStyle(::Type{<:LinearAlgebra.AdjointQ}) = DefaultArrayStyle{2}()it would be fine, but can't do that on our side without piracy. I don't really understand why they don't already do this. Solutions like defining an internal julia> using LazyArrays, LinearAlgebra
julia> B = LazyArrays.CachedArray(randn(3, 3)); Q = qr(randn(3, 3)).Q; collect(B); Q * B ≈ Q * B.data
ERROR: MethodError: no method matching ndims(::Type{LinearAlgebra.QRCompactWYQ{Float64, Matrix{Float64}, Matrix{Float64}}})
The function `ndims` exists, but no method is defined for this combination of argument types.
Closest candidates are:
ndims(::Type{Union{}}, Any...)
@ Base abstractarray.jl:276
ndims(::Type{<:Ref})
@ Base refpointer.jl:104
ndims(::Type{<:Applied{<:Any, typeof(*), Args}}) where Args
@ LazyArrays c:\Users\djv23\.julia\dev\LazyArrays.jl\src\linalg\mul.jl:41
...
Stacktrace:
[1] Base.Broadcast.BroadcastStyle(::Type{LinearAlgebra.QRCompactWYQ{Float64, Matrix{Float64}, Matrix{Float64}}})
@ Base.Broadcast .\broadcast.jl:103
[2] tuple_type_broadcastlayout
@ c:\Users\djv23\.julia\dev\LazyArrays.jl\src\lazyconcat.jl:445 [inlined]
[3] applybroadcaststyle(::Type{ApplyArray{Float64, 2, typeof(*), Tuple{…}}}, ::ApplyLayout{typeof(*)})
@ LazyArrays c:\Users\djv23\.julia\dev\LazyArrays.jl\src\lazyapplying.jl:334
[4] Base.Broadcast.BroadcastStyle(M::Type{ApplyArray{Float64, 2, typeof(*), Tuple{…}}})
@ LazyArrays c:\Users\djv23\.julia\dev\LazyArrays.jl\src\lazyapplying.jl:336
[5] combine_styles(c::ApplyArray{Float64, 2, typeof(*), Tuple{LinearAlgebra.QRCompactWYQ{…}, CachedArray{…}}})
@ Base.Broadcast .\broadcast.jl:433
[6] combine_styles(c1::ApplyArray{Float64, 2, typeof(*), Tuple{…}}, c2::Matrix{Float64})
@ Base.Broadcast .\broadcast.jl:438
[7] broadcasted
@ .\broadcast.jl:1353 [inlined]
[8] broadcast_preserving_zero_d
@ .\broadcast.jl:882 [inlined]
[9] -(A::ApplyArray{Float64, 2, typeof(*), Tuple{LinearAlgebra.QRCompactWYQ{…}, CachedArray{…}}}, B::Matrix{Float64})
@ Base .\arraymath.jl:8
[10] isapprox(x::ApplyArray{…}, y::Matrix{…}; atol::Int64, rtol::Float64, nans::Bool, norm::typeof(norm))
@ LinearAlgebra C:\Users\djv23\.julia\juliaup\julia-1.12.1+0.x64.w64.mingw32\share\julia\stdlib\v1.12\LinearAlgebra\src\generic.jl:1982
[11] isapprox(x::ApplyArray{Float64, 2, typeof(*), Tuple{LinearAlgebra.QRCompactWYQ{…}, CachedArray{…}}}, y::Matrix{Float64})
@ LinearAlgebra C:\Users\djv23\.julia\juliaup\julia-1.12.1+0.x64.w64.mingw32\share\julia\stdlib\v1.12\LinearAlgebra\src\generic.jl:1978
[12] top-level scope
@ REPL[100]:1
Some type information was truncated. Use `show(err)` to see complete types. |
…/BoundsError issue with cacheddata storing a Vcat+scalar
|
I think
is probably the best way around this? I have played around in the past with making a type so that Q's are |
|
I'm running into a bit of an annoying problem. The problem stems from trying to support First I'll just explain a new function To properly get the Lines 622 to 701 in 78b3c33
and called e.g. for LazyArrays.jl/src/lazyapplying.jl Lines 231 to 235 in 78b3c33
The code for LazyArrays.jl/test/cachetests.jl Lines 630 to 713 in 78b3c33
to try and see all the cases I was supporting so far if you did want to understand why it has to be so complex (not that there isn't probably a better way, which hopefully we can discuss). Some problems:
julia> A, b = rand(2, 2), rand(2);
julia> M = BroadcastMatrix(*, cache(A), cache(b));
julia> LazyArrays.cacheddata(M)
ERROR: ArgumentError: Cannot conform arrays with incompatible dimensions: ((2, 2), (2,))
Stacktrace:
[1] conforming_resize!
@ c:\Users\djv23\.julia\dev\LazyArrays.jl\src\cache.jl:698 [inlined]
[2] cacheddata(A::BroadcastMatrix{Float64, typeof(*), Tuple{Matrix{Float64}, Vector{Float64}}})
@ LazyArrays c:\Users\djv23\.julia\dev\LazyArrays.jl\src\lazybroadcasting.jl:127
[3] top-level scope
@ REPL[167]:1and I'm not seeing the way to fix that currently.
julia> A = Vcat((1:∞)', cache(1:∞)');
julia> B = view(A, :, 1:10);
julia> LazyArrays.cacheddata(B)
ERROR: ArgumentError: Cannot create infinite Array
Stacktrace:
[1] Vector{Int64}(::UndefInitializer, ::Tuple{Infinities.InfiniteCardinal{0}})
@ InfiniteArrays C:\Users\djv23\.julia\packages\InfiniteArrays\v6xnl\src\infarrays.jl:19
[2] Vector{Int64}(x::LazyArrays.CachedArray{Int64, 1, Vector{Int64}, Zeros{Int64, 1, Tuple{InfiniteArrays.OneToInf{…}}}})
@ Base .\array.jl:621
[3] convert(::Type{Vector{Int64}}, a::LazyArrays.CachedArray{Int64, 1, Vector{Int64}, Zeros{Int64, 1, Tuple{…}}})
@ Base .\array.jl:614
[4] setproperty!(x::LazyArrays.CachedArray{…}, f::Symbol, v::LazyArrays.CachedArray{…})
@ Base .\Base_compiler.jl:57
[5] _vec_resizedata!(B::LazyArrays.CachedArray{…}, n::Infinities.InfiniteCardinal{…})
@ LazyArrays c:\Users\djv23\.julia\dev\LazyArrays.jl\src\cache.jl:254
[6] resizedata!(::DenseColumnMajor, ::LazyArrays.LazyLayout, B::LazyArrays.CachedArray{…}, n::Infinities.InfiniteCardinal{…})
@ LazyArrays c:\Users\djv23\.julia\dev\LazyArrays.jl\src\cache.jl:266
[7] resizedata!(B::LazyArrays.CachedArray{…}, mn::Infinities.InfiniteCardinal{…})
@ LazyArrays c:\Users\djv23\.julia\dev\LazyArrays.jl\src\cache.jl:229
[8] resizedata!(A::Adjoint{Int64, LazyArrays.CachedArray{…}}, m::Int64, n::Infinities.InfiniteCardinal{0})
@ LazyArrays c:\Users\djv23\.julia\dev\LazyArrays.jl\src\cache.jl:237
[9] _resize_each!
@ c:\Users\djv23\.julia\dev\LazyArrays.jl\src\cache.jl:711 [inlined]
[10] _resize_each!
@ c:\Users\djv23\.julia\dev\LazyArrays.jl\src\cache.jl:712 [inlined]
[11] conforming_resize!(args::Tuple{Adjoint{…}, Adjoint{…}})
@ LazyArrays c:\Users\djv23\.julia\dev\LazyArrays.jl\src\cache.jl:702
[12] cacheddata(A::ApplyArray{Int64, 2, typeof(vcat), Tuple{Adjoint{…}, Adjoint{…}}})
@ LazyArrays c:\Users\djv23\.julia\dev\LazyArrays.jl\src\lazyapplying.jl:233
[13] cacheddata(V::SubArray{Int64, 2, ApplyArray{…}, Tuple{…}, false})
@ LazyArrays c:\Users\djv23\.julia\dev\LazyArrays.jl\src\cache.jl:617
[14] top-level scope
@ REPL[172]:1
Some type information was truncated. Use `show(err)` to see complete types.this example fails because it wants to create There are a few ideas I have, like trying to reimplement |
…dispatch for _bc_resizecacheddata! rather than a MemoryLayout dispatch
Now going to use this PR to get various other fixes for propagating CachedArrayStyle. Makes it easier for me when debugging but if it's preferred I can later break it up into smaller PRs. Once all the changes are in, I'll edit in all the new features here. Original PR text below.
This changes
VcatandHcatto use the combinedBroadcastStylebetween aLazyArrayStyleand the combined style of its arguments. This fixes e.g.Vcatof cached arguments not being recognised as aCachedArrayStyle.The implementation needs to use some recursion to get the
BroadcastStyleof the arguments viatuple_type_broadcastlayout, but basically it's just doing the same as what you'd get fromresult_style(LazyArrayStyle{N}(), combine_style(A.args...))whereAis aVcat/Hcat